Skip to content

Remove bevy_ptr::dangling_with_align() and inline it #21822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
BD103 wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Remove bevy_ptr::dangling_with_align() and inline it #21822

BD103 wants to merge 8 commits into from
+36 −30

Conversation

@BD103
Copy link
Member

@BD103 BD103 commented Nov 13, 2025 •
edited
Loading

Objective

  • bevy_ptr::dangling_with_align() is only used once, in bevy_ecs's BlobArray::with_capacity(), and it isn't generally useful outside of the engine's internals. We can remove the function and inline its implementation into its call site.
  • Additionally, bevy_ptr::dangling_with_align() has a TODO comment that was leftover from Remove int2ptr cast in bevy_ptr::dangling_with_align and remove -Zmiri-permissive-provenance in CI #15311 (comment), where it was suggested that dangling_with_align() should use without_provenance().
    • with_addr(), mentioned in the TODO comment, could also be used, but it's a more roundabout solution. The reason it was mentioned was because the original author thought it would be stabilized before without_provenance().

Solution

  • Remove dangling_with_align().
  • Replace its usage with NonNull::without_provenance() (since it is now stable and doesn't require unsafe or pointer math)

Testing

  • I ran Miri with strict provenance checking enabled to ensure the behavior is maintained.
    • MIRIFLAGS="-Zmiri-strict-provenance" cargo +nightly miri test

But what is this provenance thingy?

The official docs provide a more in-depth explanation, but basically a pointer is more than just a number referring to a memory address. Pointers have permissions associated with them that track:

  • What set of memory addresses are allowed to be accessed
  • When the pointer is allowed to access those addresses
  • If the pointer is allowed to mutate the memory, or just read it

These permissions are pointer provenance. They aren't stored at runtime, the Rust compiler doesn't know them at compile time! Miri is the only tool that I know of that tracks provenance, which it uses to ensure pointers adheres to their spatial, temporal, and mutability permissions. That's why I mentioned Miri in the Testing section. :)

@BD103 BD103 added C-Code-Quality A section of code that is hard to understand or change A-Pointers Relating to Bevy pointer abstractions D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 13, 2025
@alice-i-cecile alice-i-cecile requested a review from hymm November 13, 2025 01:17
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 13, 2025

@SkiFire13, want to take a look at this? This was your suggestion I believe.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 13, 2025

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@BD103
Copy link
Member Author

BD103 commented Nov 13, 2025

NonNull::without_provenance() was stabilized in 1.89.0, but our MSRV was 1.88.0, so I bumped it!

@hymm
Copy link
Contributor

hymm commented Nov 13, 2025 •
edited
Loading

why do we care that the alignment is a power of 2? i.e. why don't we just use NonNull::without_provenance directly?

I only see it being used in bevy here.

bevy/crates/bevy_ecs/src/storage/blob_array.rs

Line 45 in 2a24658

let data = bevy_ptr::dangling_with_align(align);

SkiFire13
SkiFire13 reviewed Nov 13, 2025
View reviewed changes
@BD103
Copy link
Member Author

BD103 commented Nov 14, 2025

why do we care that the alignment is a power of 2? i.e. why don't we just use NonNull::without_provenance directly?

I don't know enough myself to speak on this matter, but it appears to be a requirement by Rust. Looking at Layout::align(), it guarantees the result to be a power of two. (And internally it looks like it only powers of two in memory layouts using the Alignment struct / enum.)

I only see it being used in bevy here.

Given how dangling_with_align() is only being used there, we should definitely inline it. I don't think anyone else will miss it if it's gone, and I'll add a migration guide if they do.

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 14, 2025
BD103 added 3 commits November 13, 2025 20:57
@BD103
refactor!: remove dangling_with_align() and inline it into its call…
612fd5d 
… site

`dangling_with_align()` is only being used once, in `bevy_ecs`, and it's only 2 lines of code! Let's squash it :)
@BD103
chore: write migration guide
7920217
@BD103
fix: new clippy lints
8f10a99
@BD103 BD103 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 14, 2025
@BD103 BD103 changed the title Use NonNull::without_provenance() in dangling_with_align() Remove bevy_ptr::dangling_with_align() and inline it Nov 14, 2025
@kfc35
Copy link
Contributor

kfc35 commented Nov 14, 2025

The lint fixes might belong in a different pull request, since it is a little distracting from the purpose listed in the PR title and description. Plus, if you need to revert for whatever reason if the version upgrade or the without_provenance stuff ends up being problematic, the lint changes won’t be tied to the reversion

@hymm hymm mentioned this pull request Dec 5, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2025
@hymm
Fix some lints from rust 1.89 bump (#22034)
700ee46 
# Objective

- In #21822 the MSRV bump triggers some new lint warnings, so this pr
fixes them.

## Solution

- Bumped the msrv in bevy_ecs and fixed the lints. But this pr does not
actually bump the msrv. These fixes are all for combining nested if's
together.
@hymm
Copy link
Contributor

hymm commented Dec 5, 2025

I fixed the lints over here #22034. So this just needs to merge main in and I think it's ready to go.

@BD103
Copy link
Member Author

BD103 commented Dec 5, 2025

Ok, merged from main!

kfc35
kfc35 approved these changes Dec 5, 2025
View reviewed changes
Copy link
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for correctness. I’ll excuse the bevy_error.rs file with lint fixes :)
(but I can’t speak to bumping up the rust-version)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hymm hymm Awaiting requested review from hymm

2 more reviewers

@SkiFire13 SkiFire13 SkiFire13 left review comments

@kfc35 kfc35 kfc35 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Pointers Relating to Bevy pointer abstractions C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BD103 @alice-i-cecile @hymm @kfc35 @SkiFire13